Skip to content

fix(edr): implement pygeoapi's instance contract - #876

Open
jirhiker wants to merge 1 commit into
chore/actively-monitored-wells-descriptionfrom
fix/edr-instances-contract
Open

fix(edr): implement pygeoapi's instance contract#876
jirhiker wants to merge 1 commit into
chore/actively-monitored-wells-descriptionfrom
fix/edr-instances-contract

Conversation

@jirhiker

@jirhiker jirhiker commented Aug 23, 2026

Copy link
Copy Markdown
Member

Stacked on #874 (which is itself stacked on #866). Base is chore/actively-monitored-wells-description; GitHub retargets as the chain merges.

The bug

GET /ogcapi/collections/waterlevels/instances returns 500:

File "pygeoapi/api/environmental_data_retrieval.py", line 125, in get_collection_edr_instances
    for instance in instances:
TypeError: 'NotImplementedError' object is not iterable

pygeoapi calls p.instances() and p.instance(id). WaterEDRProvider spelled them get_instances() and get_instance(), so it overrode nothing.

What made this silent rather than loud: BaseEDRProvider.instances/instance return a NotImplementedError instance rather than raising one. So /instances iterated that object (the TypeError above), and /instances/{id}/… validated the id against a truthy object — meaning any instance identifier was accepted, valid or not.

Transducer deployments have therefore never been reachable as EDR instances, which is precisely what the collection advertises:

Each transducer deployment is exposed as its own EDR instance, so a well's record can be read deployment by deployment or as a whole.

Renamed to the contract pygeoapi actually calls. Nothing in the repo referenced the old names.

Why CI never caught it

tests/features/edr-water-data.feature has covered this since ADR3 — "Transducer deployments are exposed as EDR instances" — and it fails on the first run. But the feature is tagged @backend @edr with no @production, and CI runs @backend and @production and not @skip. It has never run there. Now tagged @production.

Two fixture defects that were hiding behind that

Turning the feature on exposed both; the chemistry scenarios failed with 400 and 204:

  1. Chemistry was seeded into the wrong tables. The fixture writes observation rows, but d9e0f1a2b3c4 rebuilt ogc_water_chemistry over the legacy NMA_* tables, so the collection saw nothing — parameter-name=pH came back "Invalid parameter-name" (400) because the provider's field list was empty, and location queries returned 204. It now seeds NMA_Chemistry_SampleInfo / NMA_FieldParameters, and refreshes the materialized view, without which the rows stay invisible regardless.
  2. The fixture well was draft. ogc_water_chemistry gates on the thing's release_status as well as the sample's, and seeded wells default to draft, so the fixture could never publish chemistry. It now promotes its own well to public.

Verification

  • uv run behave tests/features/edr-water-data.feature8 scenarios, 0 failed (was 5 failed / 1 errored)
  • DROP_AND_REBUILD_DB=1 uv run behave tests/features --tags="@backend and @production and not @skip"9 features, 85 scenarios, 0 failed
  • uv run pytest --ignore=tests/transfers → 1098 passed, 81 skipped, 6 xpassed
  • pre-commit (black, flake8) clean

New unit tests assert the contract directly — that instances/instance are overridden and are not the base implementations — so a future rename cannot silently reintroduce this.

Flagged, not fixed here

ogc_waterlevels filters on the observation's release_status only, while ogc_water_chemistry also requires the parent thing to be public. A draft well with public readings is therefore published through EDR water levels but not through EDR chemistry. That inconsistency is a publication-policy decision, not a bug to settle in this PR — it belongs with BDMS-988 (A9, define the publication predicate per layer family).

🤖 Generated with Claude Code

/ogcapi/collections/waterlevels/instances returned a 500:

    TypeError: 'NotImplementedError' object is not iterable

pygeoapi calls p.instances() and p.instance(id); this provider spelled
them get_instances() and get_instance(). Because BaseEDRProvider
*returns* a NotImplementedError instance from both rather than raising
one, the mismatch was silent -- /instances iterated that object, and
/instances/{id}/... validated the id against a truthy object, so any
identifier at all was accepted. Transducer deployments have therefore
never been reachable as EDR instances, which is exactly what the
waterlevels description advertises. Renamed; nothing called the old
names.

The behave feature that covers this has existed since ADR3 and caught it
on the first run, but it is tagged @backend @edr with no @production, and
CI runs "@backend and @production and not @Skip" -- so it has never run
there. Tagged @production.

Two fixture defects were hiding behind that, both of which made the
chemistry scenarios fail once the feature ran:

* The fixture seeded chemistry as observation rows, but d9e0f1a2b3c4
  rebuilt ogc_water_chemistry over the legacy NMA_* tables, so the
  collection saw nothing -- a 400 (pH is not a known parameter) and a
  204. It now seeds NMA_Chemistry_SampleInfo/NMA_FieldParameters and
  refreshes the materialized view, without which the rows stay invisible
  anyway.
* ogc_water_chemistry gates on the thing's release_status as well as the
  sample's, and wells seed as 'draft', so the fixture published no
  chemistry at all. It now promotes its own well to public.

Full production behave suite: 85 scenarios, 0 failed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant